feat: support helius laser GRPC client for cloning #587
Conversation
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
WalkthroughAdds Helius Laserstream support and a multiplexing pubsub client: new Laser actor/client, ChainUpdatesClient enum (WS vs Laser), shared pubsub_common types, Endpoint helpers and GRPC detection, ChainPubsubClient.shutdown now returns Result, workspace/test tooling, and updated consumers to use ChainUpdatesClient. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Consumer (Chainlink)
participant CUC as ChainUpdatesClient
participant WS as ChainPubsubClientImpl
participant Laser as ChainLaserClientImpl
App->>CUC: try_new_from_endpoint(endpoint)
alt endpoint is Helius Laser (gRPC)
CUC->>Laser: new_from_url(pubsub_url, api_key)
Laser-->>CUC: initialized
else WebSocket
CUC->>WS: try_new_from_url(pubsub_url)
WS-->>CUC: initialized
end
App->>CUC: subscribe(pubkey)
alt using Laser
CUC->>Laser: subscribe(pubkey)
Laser->>Laser: spawn per-account streams
else using WS
CUC->>WS: subscribe(pubkey)
WS->>WS: spawn per-account streams
end
note over Laser,WS: backends emit SubscriptionUpdate(pubkey, slot, account)
Laser-->>CUC: SubscriptionUpdate
WS-->>CUC: SubscriptionUpdate
App->>CUC: shutdown()
CUC->>WS: shutdown()
CUC->>Laser: shutdown()
WS-->>CUC: Ok(())
Laser-->>CUC: Ok(())
CUC-->>App: Ok(())
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
thlorenz
force-pushed
the
thlorenz/laser-grpc
branch
from
479d26c to
ba3580b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 27
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
magicblock-chainlink/src/submux/mod.rs (5)
503-505: Potential panic: division by zero when debounce interval is 0ms.allowed_in_debounce_window_count divides detection_window by debounce_interval without guarding zero. Config can set interval_millis to 0, causing a runtime panic.
Apply a clamp to avoid zero and keep semantics predictable:
fn allowed_in_debounce_window_count(&self) -> usize { - (self.debounce_detection_window.as_millis() - / self.debounce_interval.as_millis()) as usize + let num = self.debounce_detection_window.as_millis(); + let denom = self.debounce_interval.as_millis().max(1); // clamp to 1ms + (num / denom) as usize }Also consider validating DebounceConfig at construction to reject 0ms intervals.
273-276: Don’t ignore send errors; exit tasks when the consumer is gone.Silently discarding tx.send errors can spin forwarders/flushers after the receiver drops.
Apply this to stop tasks cleanly:
- for update in to_forward { - let _ = out_tx.send(update).await; - } + for update in to_forward { + if out_tx.send(update).await.is_err() { + // Receiver dropped; terminate flusher task + return; + } + }- let _ = params.tx.send(update).await; + if params.tx.send(update).await.is_err() { + break; // receiver dropped; end forwarder + }- let _ = params.tx.send(u).await; + if params.tx.send(u).await.is_err() { + break; // receiver dropped; end forwarder + }Also applies to: 366-366, 375-375
219-230: No shutdown path for background tasks.Pruner, flusher, and recycler loops are infinite and outlive shutdown(), holding resources.
Add a CancellationToken to SubMuxClient and check it in all loops; cancel it from ChainPubsubClient::shutdown().
Also applies to: 232-278, 280-306
728-734: Inconsistent test comment about dedupe window.Comment says “dedup window is 2s,” but this test sets dedupe_window_millis: Some(100).
Update the comment to reflect the 100ms window to avoid confusion.
446-454: Avoid per-iteration allocation in debounce detection.Allocating window_slice Vec on each update adds overhead.
Iterate adjacent pairs without allocation:
- let start = len - allowed_count; - let window_slice: Vec<Instant> = - arrivals.iter().skip(start).cloned().collect(); - window_slice.windows(2).all(|w| { - let dt = w[1].saturating_duration_since(w[0]); - dt <= debounce_interval - }) + let start = len - allowed_count; + let mut ok = true; + let mut it = arrivals.iter().skip(start).copied(); + if let Some(mut prev) = it.next() { + for cur in it { + if cur.saturating_duration_since(prev) > debounce_interval { + ok = false; break; + } + prev = cur; + } + } + okmagicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs (1)
336-339: Typo in expect message.“poisonde” -> “poisoned”.
- .expect("subscription_watchers lock poisonde"); + .expect("subscription_watchers lock poisoned");magicblock-chainlink/src/remote_account_provider/mod.rs (1)
156-196: Change impl block specialization to match the actual return type for API clarityThe impl block at line 156 specializes on
SubMuxClient<ChainPubsubClientImpl>, buttry_from_urls_and_configreturnsRemoteAccountProvider<ChainRpcClientImpl, SubMuxClient<ChainUpdatesClient>>. This mismatch is confusing sinceChainUpdatesClient(an enum implementingChainPubsubClient) is what the method actually produces.Change the impl specialization to
SubMuxClient<ChainUpdatesClient>to align with the return type. The type is valid:ChainUpdatesClientimplements the requiredChainPubsubClienttrait.Verify that call sites in
magicblock-chainlink/src/chainlink/mod.rs(line 97) andtest-integration/test-chainlink/src/ixtest_context.rs(line 100) still compile and type-infer correctly after the specialization change, as they currently rely on implicit type resolution for the associated function call.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.locktest-manual/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
Cargo.toml(1 hunks)magicblock-accounts/src/scheduled_commits_processor.rs(2 hunks)magicblock-aperture/src/state/mod.rs(2 hunks)magicblock-api/src/external_config.rs(4 hunks)magicblock-api/src/magic_validator.rs(3 hunks)magicblock-chainlink/Cargo.toml(1 hunks)magicblock-chainlink/src/chainlink/mod.rs(2 hunks)magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs(1 hunks)magicblock-chainlink/src/remote_account_provider/chain_laser_client.rs(1 hunks)magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs(6 hunks)magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs(5 hunks)magicblock-chainlink/src/remote_account_provider/chain_updates_client.rs(1 hunks)magicblock-chainlink/src/remote_account_provider/errors.rs(1 hunks)magicblock-chainlink/src/remote_account_provider/mod.rs(8 hunks)magicblock-chainlink/src/remote_account_provider/pubsub_common.rs(1 hunks)magicblock-chainlink/src/submux/debounce_state.rs(1 hunks)magicblock-chainlink/src/submux/mod.rs(18 hunks)magicblock-chainlink/src/testing/chain_pubsub.rs(1 hunks)test-integration/test-chainlink/src/ixtest_context.rs(4 hunks)test-manual/Cargo.toml(1 hunks)test-manual/Makefile(1 hunks)test-manual/README.md(1 hunks)test-manual/helius-laser/Cargo.toml(1 hunks)test-manual/helius-laser/README.md(1 hunks)test-manual/helius-laser/src/main.rs(1 hunks)test-manual/rustfmt-nightly.toml(1 hunks)test-manual/rustfmt.toml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
magicblock-api/src/external_config.rs (1)
magicblock-chainlink/src/remote_account_provider/chain_laser_client.rs (1)
is_helius_laser_url(100-103)
magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs (3)
magicblock-chainlink/src/remote_account_provider/chain_laser_client.rs (3)
new_from_url(23-36)shutdown(81-86)subscribe(53-65)magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs (5)
new_from_url(49-56)new(58-87)shutdown(89-99)handle_msg(150-217)add_sub(219-289)magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (7)
new(174-184)shutdown(28-28)shutdown(59-65)shutdown(261-263)subscribe(20-23)subscribe(111-124)subscribe(241-249)
magicblock-chainlink/src/remote_account_provider/chain_updates_client.rs (3)
magicblock-chainlink/src/remote_account_provider/chain_laser_client.rs (7)
is_helius_laser_url(100-103)new_from_url(23-36)subscribe(53-65)unsubscribe(67-79)shutdown(81-86)recycle_connections(88-90)take_updates(92-97)magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (1)
try_new_from_url(44-54)magicblock-chainlink/src/remote_account_provider/mod.rs (2)
subscribe(723-735)unsubscribe(738-749)
magicblock-chainlink/src/remote_account_provider/chain_laser_client.rs (4)
magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs (3)
new_from_url(51-74)new(76-99)shutdown(101-104)magicblock-chainlink/src/remote_account_provider/mod.rs (4)
new(135-140)new(222-264)subscribe(723-735)unsubscribe(738-749)magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (16)
new(174-184)subscribe(20-23)subscribe(111-124)subscribe(241-249)unsubscribe(24-27)unsubscribe(126-139)unsubscribe(251-259)shutdown(28-28)shutdown(59-65)shutdown(261-263)recycle_connections(29-29)recycle_connections(67-97)recycle_connections(228-230)take_updates(31-31)take_updates(99-109)take_updates(232-240)magicblock-chainlink/src/remote_account_provider/chain_updates_client.rs (5)
subscribe(62-71)unsubscribe(73-82)shutdown(84-90)recycle_connections(92-98)take_updates(100-106)
magicblock-api/src/magic_validator.rs (2)
magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs (1)
new(76-99)magicblock-chainlink/src/remote_account_provider/mod.rs (2)
new(135-140)new(222-264)
magicblock-chainlink/src/submux/mod.rs (2)
magicblock-chainlink/src/remote_account_provider/chain_laser_client.rs (1)
shutdown(81-86)magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (3)
shutdown(28-28)shutdown(59-65)shutdown(261-263)
magicblock-chainlink/src/remote_account_provider/mod.rs (5)
magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs (1)
new(76-99)magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs (1)
new(58-87)magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (1)
new(174-184)magicblock-chainlink/src/submux/mod.rs (1)
new(158-169)magicblock-chainlink/src/remote_account_provider/chain_updates_client.rs (1)
try_new_from_endpoint(20-57)
magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs (3)
magicblock-chainlink/src/remote_account_provider/chain_laser_client.rs (1)
shutdown(81-86)magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (4)
shutdown(28-28)shutdown(59-65)shutdown(261-263)send(190-199)magicblock-chainlink/src/remote_account_provider/pubsub_common.rs (2)
from(43-50)rpc_response(44-44)
magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (4)
magicblock-chainlink/src/remote_account_provider/chain_laser_client.rs (1)
shutdown(81-86)magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs (1)
shutdown(89-99)magicblock-chainlink/src/submux/mod.rs (1)
shutdown(547-552)magicblock-chainlink/src/remote_account_provider/pubsub_common.rs (2)
from(43-50)rpc_response(44-44)
🪛 checkmake (0.2.2)
test-manual/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 markdownlint-cli2 (0.18.1)
test-manual/helius-laser/README.md
28-28: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
33-33: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
38-38: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
43-43: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: run_make_ci_test
- GitHub Check: run_make_ci_lint
- GitHub Check: run_make_ci_test
- GitHub Check: run_make_ci_format
🔇 Additional comments (33)
test-manual/rustfmt.toml (1)
1-2: LGTM!Standard Rustfmt configuration that aligns with the project's formatting standards.
magicblock-chainlink/Cargo.toml (1)
11-11: LGTM!Correctly adds the helius-laserstream workspace dependency to enable Laser GRPC client integration.
test-manual/rustfmt-nightly.toml (1)
1-3: LGTM!Nightly Rustfmt configuration with appropriate import organization settings. The max_width aligns with the stable configuration.
test-manual/helius-laser/Cargo.toml (1)
1-13: LGTM!Properly configured package manifest that uses workspace dependencies correctly for the manual test binary.
test-manual/README.md (1)
1-26: LGTM!Well-structured documentation that clearly explains the manual test setup, requirements, and usage. The rationale for manual testing is well articulated.
magicblock-chainlink/src/testing/chain_pubsub.rs (1)
8-9: All imported types exist in the specified modules.The rg search confirmed that
ChainPubsubActorMessage,SubscriptionUpdate, andChainPubsubActorare correctly defined in their respective modules. However, cargo check could not complete in the sandbox environment due to Rust toolchain issues.Please verify locally that imports compile:
cargo check --testsmagicblock-chainlink/src/submux/debounce_state.rs (1)
5-5: Import path update looks correctThe shared pubsub_common::SubscriptionUpdate import aligns with the refactor.
magicblock-chainlink/src/remote_account_provider/errors.rs (1)
32-37: Good extension of error surface; verify propagation pathsVariants look fine. Ensure all laser actor send sites map to ChainLaserActorSendError, and API key validation surfaces MissingApiKey early (constructor/wiring).
Would you like a quick grep script to verify all laser send paths and API-key checks route to these variants?
magicblock-api/src/external_config.rs (1)
4-4: Import looks correctImporting is_helius_laser_url for GRPC detection is appropriate.
magicblock-accounts/src/scheduled_commits_processor.rs (1)
13-14: Type alias migration LGTMSwapping ChainPubsubClientImpl → ChainUpdatesClient via SubMuxClient matches the new backend. No issues spotted.
Also applies to: 51-54
magicblock-aperture/src/state/mod.rs (1)
10-11: Consistent wiring to ChainUpdatesClientAlias and imports updated correctly. Matches the rest of the PR.
Also applies to: 24-27
magicblock-chainlink/src/chainlink/mod.rs (1)
20-22: Return type and imports updated correctlyType now returns SubMuxClient. Constructor flow remains valid.
Also applies to: 92-93
magicblock-api/src/magic_validator.rs (1)
27-28: Wiring to ChainUpdatesClient and Endpoint::new is correctType alias and endpoint construction look good and align with backend selection.
Also applies to: 99-104, 395-397
magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (5)
9-13: Import path adjustments are correctSubscriptionUpdate/ActorMessage sourced from pubsub_common matches relocations.
28-29: Trait change: shutdown now returns ResultGood API: callers can observe shutdown failures; SubMux already awaits and aggregates.
Please confirm all implementors were updated (Laser, SubMux). CI should catch any missing impls.
59-65: Graceful shutdown path LGTMSends Shutdown and awaits ack; propagates errors.
221-223: SubscriptionUpdate::from use is correctMatches new constructor in pubsub_common, avoids manual struct init.
261-263: Mock shutdown returning Ok(()) is fineKeeps tests/dev-context simple and compatible with new trait.
magicblock-chainlink/src/remote_account_provider/chain_updates_client.rs (1)
60-107: Delegation to inner client variants is straightforwardsubscribe/unsubscribe/shutdown/recycle/take_updates all forward correctly.
magicblock-chainlink/src/submux/mod.rs (3)
15-16: Imports and type surface alignment look correct.Using pubsub_common::SubscriptionUpdate and errors::RemoteAccountProviderResult is consistent with the refactor across the R.A.P. surface.
356-357: Correct dedup key uses update.slot.Switching from rpc_response.context.slot to update.slot matches the new SubscriptionUpdate shape.
635-637: Tests: updates to lamports accessor, slot assertions, and shutdown await all look good.Please keep CI timeouts generous; several 100ms timeouts can be flaky under load. Consider 2–3x slack where possible.
Also applies to: 641-642, 685-686, 749-750, 811-812, 893-893, 1002-1003, 1040-1040, 1100-1100, 1141-1142, 1186-1187, 1200-1201
magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs (2)
176-196: Unsubscribe via drop: verify semantics.remove_sub drops the per-account stream; create_account_stream doesn’t keep an explicit unsubscribe handle.
Confirm laserstream unsubscribes server‑side when the stream is dropped. If not, carry and call an explicit unsubscribe.
Also applies to: 163-174
198-266: Account update handling looks robust.Good pubkey consistency check, owner parsing, and construction of SubscriptionUpdate with slot.
magicblock-chainlink/src/remote_account_provider/chain_laser_client.rs (1)
22-36: Client/actor wiring looks sound.Construction, message passing with oneshots, shutdown, and take_updates mirror existing pubsub client patterns.
Also applies to: 51-98
magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs (2)
274-279: Using SubscriptionUpdate::from tuple is correct and clearer.
209-216: Shutdown flow integration looks good.Gracefully cancels subs and signals via oneshot; worker loop respects cancellation.
Also applies to: 89-99, 127-147
magicblock-chainlink/src/remote_account_provider/pubsub_common.rs (2)
42-51: SubscriptionUpdate::from correctly decodes UiAccount and captures slot.
67-83: Actor message surface looks appropriate.Includes Shutdown and RecycleConnections responses as Result<()>; consistent with callers.
test-integration/test-chainlink/src/ixtest_context.rs (1)
32-32: Import path change to crate::sleep_msLooks fine assuming re-export; otherwise this will fail in tests.
Confirm the symbol is re-exported at crate::sleep_ms.
magicblock-chainlink/src/remote_account_provider/mod.rs (3)
39-41: Module surface updatesVisibility choices look good (actor kept crate-private; clients public).
Also applies to: 44-44, 49-49
54-61: Import consolidation for new updates pathGood alignment with ChainUpdatesClient and SubscriptionUpdate.
291-299: Client construction via ChainUpdatesClient::try_new_from_endpointWiring for Laser vs WebSocket selection is clear; submux over mixed clients is fine.
Also applies to: 303-305
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
magicblock-api/src/external_config.rs (1)
90-101: Panic messages reference wrong variant nameIn CustomWithWs arm, two expects mention CustomWithMultipleWs. Fix messages.
- .expect("rpc url must be set for CustomWithMultipleWs") + .expect("rpc url must be set for CustomWithWs") ... - .expect("ws urls must be set for CustomWithMultipleWs") + .expect("ws urls must be set for CustomWithWs")magicblock-chainlink/src/submux/mod.rs (1)
502-505: Bug: debounce can enable immediately when detection_window < intervalallowed_in_debounce_window_count returns 0 in that case, causing immediate debouncing on first arrival. Clamp to a safe minimum and require at least one interval (two arrivals).
Apply:
- fn allowed_in_debounce_window_count(&self) -> usize { - (self.debounce_detection_window.as_millis() - / self.debounce_interval.as_millis()) as usize - } + fn allowed_in_debounce_window_count(&self) -> usize { + let intervals = (self.debounce_detection_window.as_millis() + / self.debounce_interval.as_millis()) as usize; + // Require at least 2 arrivals (>=1 interval) to consider enabling debounce. + std::cmp::max(2, intervals.saturating_add(1)) + }Consider adding a unit test configuring detection_window < interval to assert no immediate debounce.
♻️ Duplicate comments (24)
Cargo.toml (1)
80-80: Use git rev instead of branch for reproducible builds.The
helius-laserstreamdependency uses a git branch reference, which can move and reduce build reproducibility. Per the prior review, this should be pinned to a specific commit.Apply this fix:
-helius-laserstream = { git = "https://github.com/magicblock-labs/laserstream-sdk", branch = "v0.2.2-magicblock" } +helius-laserstream = { git = "https://github.com/magicblock-labs/laserstream-sdk", rev = "c3e42c5424d317093ce3ce3652d0c9c998f52eba" }test-manual/Cargo.toml (2)
13-13: Use git rev instead of branch for reproducible builds.The
helius-laserstreamdependency uses a git branch reference. This should be pinned to the commit hash for reproducibility, matching the fix needed in the root Cargo.toml.Apply this fix:
-helius-laserstream = { git = "https://github.com/magicblock-labs/laserstream-sdk", branch = "v0.2.2-magicblock" } +helius-laserstream = { git = "https://github.com/magicblock-labs/laserstream-sdk", rev = "c3e42c5424d317093ce3ce3652d0c9c998f52eba" }
21-23: Move [[bin]] to package manifest; remove from workspace.Binary targets cannot be defined in workspace manifests. Cargo only recognizes
[patch],[replace], and[profile.*]sections at the workspace level. The[[bin]]section must be moved tohelius-laser/Cargo.toml.Remove this block from test-manual/Cargo.toml and add it to helius-laser/Cargo.toml:
[[bin]] name = "helius-laser" path = "src/main.rs"test-manual/helius-laser/README.md (1)
28-30: Add blank lines around fenced code blocks (MD031).Fenced code blocks must be surrounded by blank lines for markdown lint compliance. This was flagged in the prior review.
Apply this diff to all code blocks:
1. Ensure you have a Helius API key set in the `HELIUS_API_KEY` environment variable: + ```bash export HELIUS_API_KEY=your_helius_api_key_here ``` + 2. Ensure your Solana CLI is configured for devnet: + ```bash solana config set --url devnet ``` + 3. Airdrop some SOL to your configured keypair: + ```bash solana airdrop 1 ``` + 4. Run the test from the `test-manual` directory: + ```bash make manual-test-laser ```Also applies to: 33-35, 38-40, 43-45
test-manual/Makefile (4)
1-8: Declare .PHONY targets to avoid file name collisions.The fmt, list, and manual-test-laser targets are meta targets that should be declared as .PHONY to prevent conflicts with files of the same name and satisfy checkmake requirements.
26-34: Security: API key exposed in world-readable config file.The config file
/tmp/mb-test-laser.tomlcontainsHELIUS_API_KEYand is created with default permissions (likely 0644), making it readable by all users. Additionally, the file is not removed during cleanup at lines 61-65.
44-46: Infinite wait if validator fails to start.The loop waits indefinitely for the validator to respond. If startup fails, the Makefile hangs with no diagnostic output or timeout.
49-59: Exit status check is broken: cargo failures not detected.Each Makefile recipe line runs in a separate shell, so
$?at line 53 reflects the previous@echo(line 52), not thecargo runcommand (lines 49-51). Failures in the laser test will be silently ignored.test-manual/helius-laser/src/main.rs (5)
9-18: Missing Message import for accurate fee calculation.The
Messagetype is needed to compute the exact fee for the final transfer transaction (line 175). Without it, the code uses the previous transaction's fee, which may cause the final transfer to fail if the actual fee differs.
81-88: Brittle balance assertions without retry logic.The immediate
assert_eq!checks can fail if the local validator lags behind the remote RPC. A bounded retry loop would reduce flakiness.
166-198: Final transfer uses wrong fee and has unwrap panic risk.Line 175 uses
unwrap()on transaction metadata (panic risk), and line 176 subtracts the previous transaction's fee instead of computing the fee for the upcoming final transfer. This can cause the final transfer to fail if fees differ.
227-261: Remove unnecessary async from synchronous function.
request_account_infoscontains noawaitexpressions and should be a plainfn. The.awaitcall at line 136 should also be removed.
274-297: Error handling masks non-404 errors from Helius.Lines 279-286 treat any Helius error as "account not found" and create a placeholder Account. This can mask network failures, authentication errors, or rate limiting issues.
magicblock-api/src/external_config.rs (4)
102-105: Avoid extra allocation; pass &str and build vector without re-stringifyingUse the existing String; no need for to_string().
- let is_grpc = is_grpc_url(&ws_url.to_string()); - let ws_urls = - vec![ws_url; if is_grpc { 1 } else { WS_MULTIPLEX_COUNT }]; + let is_grpc = is_grpc_url(ws_url.as_str()); + let ws_urls = if is_grpc { + vec![ws_url] + } else { + vec![ws_url.clone(); WS_MULTIPLEX_COUNT] + };
108-110: Fix typo in comment: “multple” → “multiple”Keeps docs clean.
- // NOTE: we assume that if multple ws urls are provided the user wants + // NOTE: we assume that if multiple ws urls are provided the user wants
157-159: Thin wrapper: inline or rename for clarityis_grpc_url just forwards to is_helius_laser_url; consider inlining or renaming to is_pubsub_grpc_url.
63-85: Port increment can overflow; use checked_add and set only on successIf port is 65535, port + 1 panics in debug or wraps in release. Make it safe.
- if let Some(port) = ws_url.port() { - ws_url - .set_port(Some(port + 1)) - .expect("valid url with port"); - } + if let Some(port) = ws_url.port() { + if let Some(next) = port.checked_add(1) { + ws_url + .set_port(Some(next)) + .expect("valid url with port"); + } // else: skip changing the port on overflow + }magicblock-chainlink/src/remote_account_provider/chain_updates_client.rs (1)
24-31: Tighten Laser endpoint detection (host/path check, not substring)Substring matching can false-positive. Parse URL and check host/path.
Proposed change in chain_laser_client.rs:
-pub fn is_helius_laser_url(url: &str) -> bool { - // Example: https://laserstream-devnet-ewr.helius-rpc.com - url.contains("laserstream") && url.contains("helius-rpc.com") -} +pub fn is_helius_laser_url(url: &str) -> bool { + if let Ok(parsed) = url::Url::parse(url) { + let host = parsed.host_str().unwrap_or_default(); + let path = parsed.path(); + (host.ends_with("helius-rpc.com") || host.ends_with(".helius-rpc.com")) + && (host.contains("laser") || path.contains("laser")) + } else { + false + } +}Remember to add
url = "2"in Cargo.toml if not present.magicblock-chainlink/src/remote_account_provider/chain_laser_client.rs (1)
100-103: Make endpoint detection more robust by parsing the URL.Substring matching can misclassify URLs. Parse the URL and check the host component.
Apply this diff to use proper URL parsing:
+use url::Url; + -pub fn is_helius_laser_url(url: &str) -> bool { - // Example: https://laserstream-devnet-ewr.helius-rpc.com - url.contains("laserstream") && url.contains("helius-rpc.com") -} +pub fn is_helius_laser_url(url: &str) -> bool { + Url::parse(url) + .ok() + .and_then(|u| u.host_str()) + .map(|host| host.contains("laserstream") && host.ends_with("helius-rpc.com")) + .unwrap_or(false) +}This ensures detection is based on the hostname, not arbitrary URL components.
magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs (2)
40-41: Update stale doc comment.The comment references
[Self::pubsub_client]which doesn't exist in this struct. Update to refer to the Laser client subscription mechanism.Apply this diff:
- /// Sends updates for any account subscription that is received via - /// the [Self::pubsub_client] + /// Sends updates for any account subscription that is received via + /// the Laser client subscriptions
47-48: Remove unsafe Send/Sync impls - they are unsound and unnecessary.The
unsafe impl Syncis unsound becauseStreamMap<Pubkey, LaserStream>contains streams that are onlySend, notSync. Additionally,tokio::spawnonly requiresSend, notSync, making these impls unnecessary.Apply this diff:
-unsafe impl Send for ChainLaserActor {} -unsafe impl Sync for ChainLaserActor {}Rely on Rust's auto-traits. If compilation fails, investigate which field blocks
Sendrather than adding unsafe impls.test-integration/test-chainlink/src/ixtest_context.rs (1)
284-287: Use expect with context instead of bare unwrap.In test code,
.expect("undelegation_requested failed")provides clearer failure messages than.unwrap().Apply this diff:
self.chainlink .undelegation_requested(counter_pda) .await - .unwrap(); + .expect("undelegation_requested failed");magicblock-chainlink/src/remote_account_provider/mod.rs (2)
142-153: Make API key extraction robust to query parameter ordering.The current
split_once("?api-key=")fails whenapi-keyis not the first query parameter. Parse the query string properly to handle arbitrary parameter order.Apply this diff:
pub fn separate_pubsub_url_and_api_key(&self) -> (String, Option<String>) { - let (pubsub_url, pubsub_api_key) = self + let (base, query) = self .pubsub_url - .split_once("?api-key=") - .unwrap_or((&self.pubsub_url, "")); - let api_key = if !pubsub_api_key.is_empty() { - Some(pubsub_api_key.to_string()) - } else { - None - }; - (pubsub_url.to_string(), api_key) + .split_once('?') + .unwrap_or((self.pubsub_url.as_str(), "")); + let api_key = query + .split('&') + .find_map(|kv| kv.strip_prefix("api-key=").map(|v| v.to_string())); + (base.to_string(), api_key) }This handles URLs like
wss://host/stream?foo=1&api-key=KEY&bar=2correctly.
364-377: Clarify error message for missing account in update.The error message "could not be decoded" is misleading since decoding happens during
SubscriptionUpdateconstruction. TheNonecase here means the account data is missing or decode failed earlier.Apply this diff to improve clarity:
let remote_account = match update.account { Some(account) => RemoteAccount::from_fresh_account( account, slot, RemoteAccountUpdateSource::Subscription, ), None => { error!( - "Account for {} update could not be decoded", - update.pubkey + "Account for {} missing in subscription update at slot {}", + update.pubkey, slot ); RemoteAccount::NotFound(slot) } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.locktest-manual/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
Cargo.toml(1 hunks)magicblock-accounts/src/scheduled_commits_processor.rs(2 hunks)magicblock-aperture/src/state/mod.rs(2 hunks)magicblock-api/src/external_config.rs(4 hunks)magicblock-api/src/magic_validator.rs(3 hunks)magicblock-chainlink/Cargo.toml(1 hunks)magicblock-chainlink/src/chainlink/mod.rs(2 hunks)magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs(1 hunks)magicblock-chainlink/src/remote_account_provider/chain_laser_client.rs(1 hunks)magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs(6 hunks)magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs(5 hunks)magicblock-chainlink/src/remote_account_provider/chain_updates_client.rs(1 hunks)magicblock-chainlink/src/remote_account_provider/errors.rs(1 hunks)magicblock-chainlink/src/remote_account_provider/mod.rs(8 hunks)magicblock-chainlink/src/remote_account_provider/pubsub_common.rs(1 hunks)magicblock-chainlink/src/submux/debounce_state.rs(1 hunks)magicblock-chainlink/src/submux/mod.rs(18 hunks)magicblock-chainlink/src/testing/chain_pubsub.rs(1 hunks)test-integration/test-chainlink/src/ixtest_context.rs(4 hunks)test-manual/Cargo.toml(1 hunks)test-manual/Makefile(1 hunks)test-manual/README.md(1 hunks)test-manual/helius-laser/Cargo.toml(1 hunks)test-manual/helius-laser/README.md(1 hunks)test-manual/helius-laser/src/main.rs(1 hunks)test-manual/rustfmt-nightly.toml(1 hunks)test-manual/rustfmt.toml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
PR: magicblock-labs/magicblock-validator#578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-chainlink/src/remote_account_provider/mod.rs
🧬 Code graph analysis (9)
magicblock-chainlink/src/remote_account_provider/chain_updates_client.rs (3)
magicblock-chainlink/src/remote_account_provider/chain_laser_client.rs (7)
is_helius_laser_url(100-103)new_from_url(23-36)subscribe(53-65)unsubscribe(67-79)shutdown(81-86)recycle_connections(88-90)take_updates(92-97)magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (1)
try_new_from_url(44-54)magicblock-chainlink/src/remote_account_provider/mod.rs (2)
subscribe(723-735)unsubscribe(738-749)
magicblock-api/src/external_config.rs (1)
magicblock-chainlink/src/remote_account_provider/chain_laser_client.rs (1)
is_helius_laser_url(100-103)
magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs (3)
magicblock-chainlink/src/remote_account_provider/chain_laser_client.rs (3)
new_from_url(23-36)shutdown(81-86)subscribe(53-65)magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs (5)
new_from_url(49-56)new(58-87)shutdown(89-99)handle_msg(150-217)add_sub(219-289)magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (7)
new(174-184)shutdown(28-28)shutdown(59-65)shutdown(261-263)subscribe(20-23)subscribe(111-124)subscribe(241-249)
magicblock-chainlink/src/remote_account_provider/chain_laser_client.rs (4)
magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs (2)
new_from_url(51-74)new(76-99)magicblock-chainlink/src/remote_account_provider/mod.rs (3)
new(135-140)new(222-264)subscribe(723-735)magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (7)
new(174-184)subscribe(20-23)subscribe(111-124)subscribe(241-249)take_updates(31-31)take_updates(99-109)take_updates(232-240)magicblock-chainlink/src/remote_account_provider/chain_updates_client.rs (2)
subscribe(62-71)take_updates(100-106)
magicblock-chainlink/src/remote_account_provider/mod.rs (5)
magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs (1)
new(76-99)magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs (1)
new(58-87)magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (1)
new(174-184)magicblock-chainlink/src/submux/mod.rs (1)
new(158-169)magicblock-chainlink/src/remote_account_provider/chain_updates_client.rs (1)
try_new_from_endpoint(20-57)
magicblock-chainlink/src/submux/mod.rs (3)
magicblock-chainlink/src/remote_account_provider/chain_laser_client.rs (1)
shutdown(81-86)magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs (1)
shutdown(89-99)magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (3)
shutdown(28-28)shutdown(59-65)shutdown(261-263)
magicblock-api/src/magic_validator.rs (1)
magicblock-chainlink/src/remote_account_provider/mod.rs (2)
new(135-140)new(222-264)
magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (4)
magicblock-chainlink/src/remote_account_provider/chain_laser_client.rs (1)
shutdown(81-86)magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs (1)
shutdown(89-99)magicblock-chainlink/src/submux/mod.rs (1)
shutdown(547-552)magicblock-chainlink/src/remote_account_provider/pubsub_common.rs (2)
from(43-50)rpc_response(44-44)
magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs (3)
magicblock-chainlink/src/remote_account_provider/chain_laser_client.rs (1)
shutdown(81-86)magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (4)
shutdown(28-28)shutdown(59-65)shutdown(261-263)send(190-199)magicblock-chainlink/src/remote_account_provider/pubsub_common.rs (2)
from(43-50)rpc_response(44-44)
🪛 checkmake (0.2.2)
test-manual/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 markdownlint-cli2 (0.18.1)
test-manual/helius-laser/README.md
28-28: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
33-33: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
38-38: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
43-43: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run_make_ci_lint
- GitHub Check: run_make_ci_test
- GitHub Check: run_make_ci_test
🔇 Additional comments (51)
test-manual/rustfmt.toml (1)
1-2: Configuration looks good.Standard rustfmt settings appropriate for the test-manual workspace. This complements the nightly-specific configuration.
test-manual/rustfmt-nightly.toml (1)
1-3: Configuration is valid.Nightly-specific settings for import organization are appropriate. Complements the standard rustfmt.toml.
magicblock-chainlink/Cargo.toml (1)
11-11: Dependency reference is correct.The
helius-laserstreamdependency correctly references the workspace definition. However, ensure the root Cargo.toml dependency is fixed for reproducible builds (see notes on root Cargo.toml).test-manual/helius-laser/Cargo.toml (1)
1-13: Package manifest is correct.The helius-laser package manifest is well-formed. Note that it will need a
[[bin]]section added (to be moved from test-manual/Cargo.toml):[[bin]] name = "helius-laser" path = "src/main.rs"test-manual/README.md (1)
1-26: Documentation is clear and well-structured.Provides good overview of manual test purpose and requirements. Cross-references to helius-laser/README.md for detailed instructions are appropriate.
magicblock-chainlink/src/submux/debounce_state.rs (1)
5-5: Import path update is correctMatches the pubsub_common relocation. No issues.
magicblock-chainlink/src/remote_account_provider/errors.rs (1)
32-37: New error variants verified as safe; no match arm updates requiredVerification confirms the additions are sound:
MissingApiKey(String)correctly passes endpoint identifier ("Helius laser endpoint: {endpoint.pubsub_url}"), not the actual keyChainLaserActorSendError(String, String)passes error descriptions (err.to_string()and debug format), no secrets- No exhaustive match statements on
RemoteAccountProviderErrorin the codebase require updatingmagicblock-accounts/src/scheduled_commits_processor.rs (1)
13-14: ChainUpdatesClient migration verifiedThe import swap (lines 13–14) and type alias change (lines 51–54) are correct. Provider wiring verified: ChainUpdatesClient::try_new_from_endpoint is correctly called with Endpoint and CommitmentConfig for each endpoint, properly routing to WebSocket or Laser variants.
magicblock-aperture/src/state/mod.rs (1)
10-11: Migration verified: ChainUpdatesClient successfully replaces ChainPubsubClientImpl in apertureNo remaining
SubMuxClient<ChainPubsubClientImpl>aliases found in magicblock-aperture. The crate now consistently usesSubMuxClient<ChainUpdatesClient>(line 24). RemainingChainPubsubClientImplreferences are confined to the separate magicblock-chainlink crate as internal implementation details and are not part of the aperture public interface.magicblock-api/src/magic_validator.rs (3)
27-31: Import swap to ChainUpdatesClient looks correctMigration to chain_updates_client::ChainUpdatesClient aligns with the new multiplexed client. No issues spotted.
99-104: Type alias updated to SubMuxClientAlias matches the new Chainlink generic signature elsewhere. Looks good.
395-397: Endpoint construction change: confirm mixed backends in ws_urlsUsing Endpoint::new(rpc_url, pubsub_url) is fine. Please confirm remote_cluster.ws_urls may include both WebSocket and Laser URLs (incl. API keys) so ChainUpdatesClient can branch per endpoint at runtime. If Laser endpoints live elsewhere, wire them in here too.
magicblock-chainlink/src/testing/chain_pubsub.rs (1)
8-10: Test imports updated to pubsub_commonImport path changes are consistent with the refactor. All good.
magicblock-chainlink/src/chainlink/mod.rs (2)
20-22: Import surface updated to ChainUpdatesClientMatches new remote_account_provider surface. No concerns.
92-93: Public return type switched to SubMuxClientSignature aligns with new client. OK.
magicblock-chainlink/src/submux/mod.rs (4)
13-16: SubscriptionUpdate import path changeUsing remote_account_provider::pubsub_common::SubscriptionUpdate is consistent with the refactor.
349-357: Dedup keyed by (pubkey, slot) is appropriateSwitching to update.slot for dedup is correct given the new SubscriptionUpdate shape.
547-552: Shutdown now returns Result; propagate and consider timeoutsAwaiting inner client shutdown and bubbling errors is good. Add a bounded timeout to avoid indefinite hangs if an actor is stuck.
If desired, we can add a wrapper:
pub async fn shutdown_with_timeout<T: ChainPubsubClient>( client: &T, dur: std::time::Duration, ) -> RemoteAccountProviderResult<()> { match tokio::time::timeout(dur, client.shutdown()).await { Ok(r) => r, Err(_) => Err(crate::remote_account_provider::errors::RemoteAccountProviderError::ShutdownTimeout), } }
635-642: Tests updated for new update shape and shutdownAccessing lamports via update.account and slot via update.slot, plus awaiting shutdown, all align with API changes. Looks good.
Also applies to: 726-750, 797-811, 871-893, 1002-1040, 1099-1142, 1186-1201
magicblock-chainlink/src/remote_account_provider/chain_updates_client.rs (1)
13-17: Wrapper and delegation look correctEnum wrapper and ChainPubsubClient delegation are straightforward. No issues.
Also applies to: 60-107
magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (5)
10-12: LGTM! Import reorganization aligns with the new module structure.The imports correctly reference the relocated types from
pubsub_common.
28-28: LGTM! Appropriate error handling added to shutdown.The updated signature enables proper error propagation from the actor shutdown flow.
59-65: LGTM! Shutdown implementation correctly uses actor messaging pattern.The oneshot channel pattern ensures the caller waits for shutdown acknowledgement from the actor.
221-222: LGTM! Using From trait for cleaner construction.The change to use
SubscriptionUpdate::from((pubkey, rpc_response))aligns with the centralized type definitions inpubsub_common.
261-263: LGTM! Mock shutdown signature updated consistently.The mock correctly implements the updated trait signature.
magicblock-chainlink/src/remote_account_provider/chain_laser_client.rs (5)
22-36: Actor task not tracked; consider error handling.The
tokio::spawn(actor.run())at line 34 is fire-and-forget. If the actor panics or terminates unexpectedly, there's no way to detect it. Consider either:
- Storing the
JoinHandlefor debugging/monitoring purposes, or- Adding error logging within the actor's
runmethodThis is acceptable for now but may complicate diagnostics in production.
38-48: LGTM! Error mapping provides good diagnostics.The helper correctly maps channel send errors to the domain-specific
ChainLaserActorSendErrorvariant with both display and debug information.
53-79: LGTM! Subscribe/unsubscribe follow consistent actor messaging pattern.Both methods correctly use oneshot channels for synchronous request-response communication with the actor.
81-90: LGTM! Shutdown and recycle implementations are appropriate.The no-op
recycle_connectionswith trace logging is suitable for Laserstream's persistent connection model.
92-97: LGTM! take_updates correctly prevents double-take with clear panic message.The implementation matches the WebSocket client and appropriately panics on double-take to surface logic bugs early.
magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs (6)
50-99: LGTM! Constructors follow consistent pattern with WebSocket actor.The separation of
new_from_url(convenience) andnew(flexible) is well-structured, and channel sizing is appropriately centralized.
106-120: LGTM! Actor run loop is clean and well-structured.The use of
tokio::select!with conditional stream polling (if !self.subscriptions.is_empty()) prevents busy-waiting when no subscriptions are active.
122-145: LGTM! Message handling covers all cases appropriately.The no-op for
RecycleConnectionsaligns with Laserstream's persistent connection model, and shutdown response handling gracefully logs failures instead of panicking.
147-174: LGTM! Subscription management is defensive and well-logged.The warning on duplicate subscriptions (line 154) and graceful error handling with
unwrap_or_elseare good practices for debugging without crashing the actor.
176-195: LGTM! Per-account stream creation follows Laserstream API.The pattern of creating individual streams for each account enables independent subscription management and cancellation.
197-265: LGTM! Excellent defensive validation and error handling.The defensive pubkey check (lines 213-224) prevents stream cross-contamination, and the comment at lines 204-207 clearly explains the expected behavior when accounts are closed. Error handling is consistent and non-fatal throughout.
magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs (4)
7-7: LGTM! Import reorganization aligns with shared type centralization.The imports correctly reference the relocated types from
pubsub_commonmodule.Also applies to: 10-10, 17-20
89-98: LGTM! Shutdown refactor improves separation of concerns.Converting shutdown to a standalone function with explicit parameters makes the actor's shutdown semantics clearer and enables better testing.
131-131: LGTM! Shutdown handling integrated consistently.Threading the
shutdown_tokenthroughhandle_msgenables coordinated shutdown, and the newShutdownbranch correctly invokes the refactored shutdown function.Also applies to: 152-152, 209-215
274-277: LGTM! Using From trait for consistent update construction.The change aligns with the centralized
SubscriptionUpdatetype inpubsub_commonand itsFromtrait implementation.magicblock-chainlink/src/remote_account_provider/pubsub_common.rs (5)
13-29: LGTM! Clean configuration struct with convenience constructor.The
from_urlconstructor provides good ergonomics for the common case.
31-61: LGTM! Well-designed type with helpful trait implementations.The
Fromimplementation gracefully handles decode failures by settingaccounttoNone, and theDisplayimpl provides useful debugging output.
63-65: LGTM! Minimal wrapper type for subscription tracking.Encapsulating the
CancellationTokenin a named struct improves code clarity.
67-83: LGTM! Clean actor message enum with consistent response pattern.All variants include oneshot response channels for synchronous request-response semantics, which is the correct pattern for actor communication.
85-86: LGTM! Centralized channel size constants.The buffer sizes (5,000 for updates, 1,000 for messages) are reasonable for high-throughput scenarios and centralizing them ensures consistency across actors.
test-integration/test-chainlink/src/ixtest_context.rs (3)
54-55: LGTM! New public fields improve test ergonomics.Exposing
clonerandvalidator_kpin the test context reduces boilerplate in individual tests.
351-351: LGTM! Underscore prefix correctly indicates intentionally unused value.Renaming to
_sigfollows Rust conventions and suppresses unused variable warnings appropriately.
13-14: Verification confirms: Type migration to ChainUpdatesClient is complete within ixtest_context.rs, but 4 other files still contain the old type.The search results show remaining uses of
SubMuxClient<ChainPubsubClientImpl>in:
test-integration/test-chainlink/tests/ix_remote_account_provider.rs(lines 26, 35)magicblock-chainlink/src/remote_account_provider/mod.rs(lines 159, 952)Whether these require updates as part of this PR is unclear. Confirm whether these files are in scope for this change or if they're being addressed separately.
magicblock-chainlink/src/remote_account_provider/mod.rs (3)
39-49: LGTM! New module declarations follow appropriate visibility patterns.The laser-specific modules and
pubsub_commonare correctly exposed as public, whilechain_laser_actorremains internal.
54-61: LGTM! Import updates align with type centralization.The imports correctly reference the new
ChainUpdatesClientenum and centralizedSubscriptionUpdatetype.
171-171: LGTM! Type migration to ChainUpdatesClient is complete and consistent.The changes correctly update all type parameters and construction calls to use the new multiplexed
ChainUpdatesClienttype.Also applies to: 183-183, 274-274, 292-298
magicblock-chainlink/src/remote_account_provider/chain_updates_client.rs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-chainlink/src/remote_account_provider/mod.rs (1)
156-173: Fix impl specialization mismatch for factory helperThe impl block at lines 156-173 is specialized for
SubMuxClient<ChainPubsubClientImpl>, but the methodtry_from_urls_and_configreturnsRemoteAccountProvider<..., SubMuxClient<ChainUpdatesClient>>.ChainUpdatesClientis a distinct enum type that implementsChainPubsubClient, not the same asChainPubsubClientImpl. This mismatch creates confusion about which impl block applies and risks method resolution issues at call sites.Change the impl specialization to match the actual return type:
-impl - RemoteAccountProvider< - ChainRpcClientImpl, - SubMuxClient<ChainPubsubClientImpl>, - > -{ +impl + RemoteAccountProvider< + ChainRpcClientImpl, + SubMuxClient<ChainUpdatesClient>, + > +{ pub async fn try_from_urls_and_config( endpoints: &[Endpoint], commitment: CommitmentConfig, subscription_forwarder: mpsc::Sender<ForwardedSubscriptionUpdate>, config: &RemoteAccountProviderConfig, ) -> ChainlinkResult< Option< RemoteAccountProvider< ChainRpcClientImpl, SubMuxClient<ChainUpdatesClient>, >, >, > { // unchanged } }The call site at
src/chainlink/mod.rs:97relies on return-type inference, so it will automatically benefit from the corrected impl.
♻️ Duplicate comments (1)
magicblock-chainlink/src/remote_account_provider/mod.rs (1)
365-377: Clarify log when update.account is None (no decoding step anymore)Message “could not be decoded” is misleading now that we no longer decode RPC values. Prefer an explicit “missing account in subscription update”.
- None => { - error!( - "Account for {} update could not be decoded", - update.pubkey - ); - RemoteAccount::NotFound(slot) - } + None => { + warn!( + "Account for {} missing in subscription update at slot {} (treating as NotFound)", + update.pubkey, slot + ); + RemoteAccount::NotFound(slot) + }As per previous review guidance. [Based on learnings]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
magicblock-chainlink/src/remote_account_provider/mod.rs(9 hunks)magicblock-chainlink/src/testing/chain_pubsub.rs(2 hunks)test-integration/test-chainlink/tests/chain_pubsub_actor.rs(3 hunks)test-integration/test-chainlink/tests/chain_pubsub_client.rs(3 hunks)test-integration/test-chainlink/tests/ix_remote_account_provider.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-26T16:53:29.771Z
Learnt from: thlorenz
PR: magicblock-labs/magicblock-validator#587
File: magicblock-chainlink/src/remote_account_provider/mod.rs:134-0
Timestamp: 2025-10-26T16:53:29.771Z
Learning: In magicblock-chainlink/src/remote_account_provider/mod.rs, the `Endpoint::separate_pubsub_url_and_api_key()` method uses `split_once("?api-key=")` because the api-key parameter is always the only query parameter right after `?`. No additional query parameter parsing is needed for this use case.
Applied to files:
magicblock-chainlink/src/remote_account_provider/mod.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
PR: magicblock-labs/magicblock-validator#578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-chainlink/src/remote_account_provider/mod.rs
🧬 Code graph analysis (3)
magicblock-chainlink/src/testing/chain_pubsub.rs (3)
magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (3)
shutdown(28-28)shutdown(59-65)shutdown(261-263)magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs (1)
shutdown(89-99)magicblock-chainlink/src/submux/mod.rs (1)
shutdown(547-552)
test-integration/test-chainlink/tests/chain_pubsub_actor.rs (1)
magicblock-chainlink/src/testing/chain_pubsub.rs (1)
shutdown(68-77)
magicblock-chainlink/src/remote_account_provider/mod.rs (5)
magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs (1)
new(76-99)magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (1)
new(174-184)magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs (1)
new(58-87)magicblock-chainlink/src/submux/mod.rs (1)
new(158-169)magicblock-chainlink/src/remote_account_provider/chain_updates_client.rs (1)
try_new_from_endpoint(20-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: run_make_ci_lint
- GitHub Check: run_make_ci_test
- GitHub Check: run_make_ci_test
- GitHub Check: run_make_ci_format
🔇 Additional comments (7)
magicblock-chainlink/src/testing/chain_pubsub.rs (2)
8-9: LGTM! Clean refactoring of imports.The import paths have been updated to reflect the centralization of pub/sub types into
pubsub_common. This improves code organization without affecting functionality.
68-77: LGTM! Well-implemented test helper.The new
shutdownhelper function follows the exact same pattern as the existing test helpers (subscribe,unsubscribe,recycle), ensuring consistency and maintainability. The implementation correctly uses a oneshot channel for acknowledgement and provides clear error messages.test-integration/test-chainlink/tests/chain_pubsub_actor.rs (1)
2-2: LGTM: updated imports and public shutdown helper usage are consistentImporting SubscriptionUpdate from pubsub_common and using testing::chain_pubsub::shutdown matches the new API. No issues spotted.
Also applies to: 5-6, 105-106, 165-166
test-integration/test-chainlink/tests/chain_pubsub_client.rs (1)
100-106: LGTM: clock deserialization and slot equalityReading Clock from account.data and asserting update.slot == clock.slot aligns with the production optimization.
magicblock-chainlink/src/remote_account_provider/mod.rs (3)
39-50: LGTM: module structure updatesAdding chain_laser_* and chain_updates_client along with pubsub_common export matches the new backend split.
54-61: LGTM: internal uses updatedSwitch to ChainUpdatesClient and pubsub_common::SubscriptionUpdate is consistent across the file.
134-154: LGTM: Endpoint helpers (document constraint)Endpoint::new and the simple api-key splitter are fine given the invariant that api-key is the only query param immediately after '?'. Consider adding a doc comment to encode this assumption.
“Based on learnings”
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
test-manual/helius-laser/src/main.rs (1)
263-300: Remove unnecessary async from compare_account_states.The function is declared
asyncbut contains noawaitcalls, adding unnecessary overhead.Apply this diff:
-async fn compare_account_states( +fn compare_account_states( helius_client: &RpcClient, local_client: &RpcClient, pubkey: &solana_sdk::pubkey::Pubkey, account_name: &str, ) -> Result<()> {And remove
.awaitfrom the call sites:compare_account_states( &remote_rpc_client, &local_rpc_client, &from_pubkey, "from", ) - .await?; + ?; compare_account_states( &remote_rpc_client, &local_rpc_client, &to_pubkey, "to", ) - .await?; + ?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs(1 hunks)magicblock-chainlink/src/remote_account_provider/mod.rs(10 hunks)test-manual/Makefile(1 hunks)test-manual/helius-laser/src/main.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: thlorenz
PR: magicblock-labs/magicblock-validator#587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
📚 Learning: 2025-10-26T16:53:29.820Z
Learnt from: thlorenz
PR: magicblock-labs/magicblock-validator#587
File: magicblock-chainlink/src/remote_account_provider/mod.rs:134-0
Timestamp: 2025-10-26T16:53:29.820Z
Learning: In magicblock-chainlink/src/remote_account_provider/mod.rs, the `Endpoint::separate_pubsub_url_and_api_key()` method uses `split_once("?api-key=")` because the api-key parameter is always the only query parameter right after `?`. No additional query parameter parsing is needed for this use case.
Applied to files:
magicblock-chainlink/src/remote_account_provider/mod.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
PR: magicblock-labs/magicblock-validator#578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-chainlink/src/remote_account_provider/mod.rs
📚 Learning: 2025-10-27T09:55:17.782Z
Learnt from: thlorenz
PR: magicblock-labs/magicblock-validator#587
File: magicblock-chainlink/src/remote_account_provider/mod.rs:296-298
Timestamp: 2025-10-27T09:55:17.782Z
Learning: In the magicblock-validator codebase, it is acceptable to log API keys in debug/info statements because logs are private and the team considers API keys not that sensitive in their security model.
Applied to files:
magicblock-chainlink/src/remote_account_provider/mod.rs
🧬 Code graph analysis (2)
magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs (5)
magicblock-chainlink/src/remote_account_provider/chain_laser_client.rs (3)
new_from_url(23-36)shutdown(81-86)subscribe(53-65)magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs (5)
new_from_url(49-56)new(58-87)shutdown(89-99)handle_msg(150-217)add_sub(219-289)magicblock-chainlink/src/remote_account_provider/mod.rs (4)
default(120-126)new(136-141)new(223-265)subscribe(721-733)magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (7)
new(174-184)shutdown(28-28)shutdown(59-65)shutdown(261-263)subscribe(20-23)subscribe(111-124)subscribe(241-249)magicblock-chainlink/src/remote_account_provider/chain_updates_client.rs (2)
shutdown(84-90)subscribe(62-71)
magicblock-chainlink/src/remote_account_provider/mod.rs (4)
magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs (1)
new(73-96)magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs (1)
new(58-87)magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (1)
new(174-184)magicblock-chainlink/src/remote_account_provider/chain_updates_client.rs (1)
try_new_from_endpoint(20-57)
🪛 checkmake (0.2.2)
test-manual/Makefile
[warning] 66-66: Missing required phony target "all"
(minphony)
[warning] 66-66: Missing required phony target "clean"
(minphony)
[warning] 66-66: Missing required phony target "test"
(minphony)
🪛 GitHub Actions: Run CI - Format
magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs
[error] 192-198: rustfmt check failed. Function signature formatting inconsistency detected by cargo +nightly fmt. Please run 'cargo +nightly fmt -- --config-path rustfmt-nightly.toml' to fix formatting in this file.
[error] Diff indicates a formatting change is needed for handle_account_update signature.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run_make_ci_lint
- GitHub Check: run_make_ci_test
- GitHub Check: run_make_ci_test
🔇 Additional comments (13)
magicblock-chainlink/src/remote_account_provider/mod.rs (6)
19-19: LGTM! Module structure supports unified pubsub abstraction.The new modules (
chain_laser_actor,chain_laser_client,chain_updates_client,pubsub_common) and updated imports properly establish the infrastructure for supporting both WebSocket and Laser gRPC clients through a unified interface. Thefutures_util::futureimport enables parallel client initialization.Also applies to: 40-50, 55-62
135-155: LGTM! Endpoint helpers are appropriate.The
new()constructor andseparate_pubsub_url_and_api_key()method provide clean API key extraction from Helius URLs. Based on learnings, thesplit_once("?api-key=")approach is acceptable since the api-key parameter is always the only query parameter in your use case.
172-172: LGTM! Type signature updates are consistent.All function signatures correctly updated from
SubMuxClient<ChainPubsubClientImpl>toSubMuxClient<ChainUpdatesClient>, maintaining consistency throughout the refactoring. The newChainUpdatesClientenum properly unifies WebSocket and Laser client handling.Also applies to: 184-184, 275-275, 302-302, 960-960
293-297: LGTM! Parallel client initialization improves startup time.The use of
future::try_join_all()properly parallelizes endpoint initialization, reducing startup time when multiple endpoints are configured. Error handling is preserved through thetry_join_allsemantics (short-circuits on first error), and results are correctly mapped toArc-wrapped clients.
346-346: LGTM! Update handling correctly adapted to unified SubscriptionUpdate.The changes properly reflect the simplified
SubscriptionUpdatestructure:
- Direct access to
update.slot(line 346) instead of nestedrpc_response.context.slot- Direct access to
update.account(lines 362-375) instead of decodingrpc_response.valueThe error message "could not be decoded" appropriately covers both client types: WebSocket decode failures from
UiAccountand missing account data from Laser streams.Also applies to: 362-375
959-966: LGTM! Test accessor maintains testing infrastructure.The new
rpc_client()accessor forRemoteAccountProvider<ChainRpcClientImpl, SubMuxClient<ChainUpdatesClient>>properly maintains test infrastructure after the type parameter changes. It follows the existing pattern established for other type combinations and is correctly cfg-gated.magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs (7)
1-28: LGTM: Imports and type aliases are well-structured.The imports are appropriate for async stream handling with Helius Laserstream, and the type aliases improve code readability. The
LaserStreamtype correctly requires onlySend(notSync) for use withtokio::spawn.
33-45: LGTM: Struct definition is correct and past issues resolved.The doc comment now correctly describes the subscription mechanism (previously flagged stale reference has been fixed), and the unsafe
Send/Syncimpls have been properly removed (relying on auto-traits).
103-117: LGTM: Event loop correctly implements the actor pattern.The
tokio::select!with the guard condition on line 112 efficiently handles messages and stream updates, preventing unnecessary polling when the subscription map is empty.
119-142: LGTM: Message handling is correct.The no-op for
RecycleConnections(line 130-132) is appropriate since Laserstream gRPC doesn't require connection recycling like WebSocket-based pubsub. Error handling properly logs failures without panicking.
160-171: LGTM: Unsubscribe logic is correct.The method cleanly removes the subscription and handles response send failures appropriately.
195-262: LGTM: Account update handling is robust.The defensive pubkey validation (lines 210-221), comprehensive error handling, and the reference to verification testing (lines 201-204) demonstrate thorough implementation. The error paths properly log without panicking.
268-277: LGTM: Commitment level conversion is correct.The exhaustive match ensures compile-time safety if commitment variants change.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
magicblock-api/src/external_config.rs (1)
102-102: Remove unnecessary String allocation.The
ws_urlis already aString, so calling.to_string()creates an unnecessary allocation.Apply this diff:
- let is_grpc = is_grpc_url(&ws_url.to_string()); + let is_grpc = is_grpc_url(&ws_url);magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs (2)
144-158: Avoid creating a stream when already subscribed.Create the stream only after confirming the key isn’t present to prevent wasted connections/allocations.
) { - let stream = self.create_account_stream(pubkey); if self.subscriptions.contains_key(&pubkey) { warn!("Already subscribed to account {}", pubkey); } else { + let stream = self.create_account_stream(pubkey); self.subscriptions.insert(pubkey, Box::pin(stream)); } sub_response.send(Ok(())).unwrap_or_else(|_| { warn!("Failed to send subscribe response for account {}", pubkey) }); }
47-71: Expose connection parameters via config (already noted).Making timeouts/keepalives/retry counts configurable would help tune reliability across envs.
Also applies to: 73-96
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
magicblock-api/src/external_config.rs(4 hunks)magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs(1 hunks)test-integration/test-chainlink/tests/chain_pubsub_client.rs(3 hunks)test-integration/test-chainlink/tests/ix_remote_account_provider.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
magicblock-api/src/external_config.rs (1)
magicblock-chainlink/src/remote_account_provider/chain_laser_client.rs (1)
is_helius_laser_url(100-103)
magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs (3)
magicblock-chainlink/src/remote_account_provider/chain_laser_client.rs (3)
new_from_url(23-36)shutdown(81-86)subscribe(53-65)magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (7)
new(174-184)shutdown(28-28)shutdown(59-65)shutdown(261-263)subscribe(20-23)subscribe(111-124)subscribe(241-249)magicblock-chainlink/src/remote_account_provider/chain_updates_client.rs (2)
shutdown(84-90)subscribe(62-71)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run_make_ci_lint
- GitHub Check: run_make_ci_test
- GitHub Check: run_make_ci_test
🔇 Additional comments (8)
test-integration/test-chainlink/tests/ix_remote_account_provider.rs (1)
6-6: LGTM! Clean migration to ChainUpdatesClient.The migration from
ChainPubsubClientImpltoChainUpdatesClientis consistent across all usage sites in this test file. The type parameters, function signatures, and instantiations all correctly reflect the new abstraction.Also applies to: 24-34
test-integration/test-chainlink/tests/chain_pubsub_client.rs (3)
9-9: LGTM! Import reflects module reorganization.The import path update to
pubsub_common::SubscriptionUpdatealigns with the PR's introduction of shared pubsub types in thepubsub_commonmodule.
39-39: LGTM! Clean pattern matching for optional account.The use of
map_or(0, |acc| acc.lamports)cleanly handles the case whenaccountisNone, returning 0 as a sensible default for lamports. This reflects the structural change fromrpc_response.valueto theaccountfield.
92-92: LGTM! Clock test correctly updated for new data structure.The test has been properly migrated to use the new
SubscriptionUpdatestructure:
- Clock data is now extracted from
update.account.datainstead ofrpc_response.value- The assertion at Line 103 validates that
update.slotmatchesclock_value.slot, which is a useful invariantThe ignored shutdown result at Line 92 was previously discussed and intentionally left as-is for test simplicity.
Also applies to: 98-103
magicblock-api/src/external_config.rs (3)
4-4: LGTM!The import is necessary for gRPC endpoint detection and properly scoped.
63-86: LGTM!The conditional logic correctly detects gRPC endpoints and bypasses WebSocket-specific multiplexing, scheme adjustment, and port increment. The use of
as_ref()on line 67 avoids unnecessary allocation.
157-159: LGTM!The abstraction layer is reasonable and provides flexibility for future extensions to gRPC detection logic.
magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs (1)
173-192: Add documentation and explicitSendtrait bound to improve signature clarity.The proposed changes are valid:
- The web search confirms that dropping the stream automatically closes the server-side subscription—no separate handle retention needed.
- The codebase already defines
type LaserStream = Pin<Box<dyn Stream<Item = LaserResult> + Send>>(line 27), and the stream fromcreate_account_streamis stored insubscriptions: StreamMap<Pubkey, LaserStream>, which requiresSend. Adding+ Sendto the return type makes this contract explicit at the function signature rather than relying on implicit conversion.- All three documentation additions clarify intent without affecting behavior.
- ) -> impl Stream<Item = LaserResult> { + ) -> impl Stream<Item = LaserResult> + Send { let mut accounts = HashMap::new(); accounts.insert( - "account_subs".into(), + // Arbitrary local label to group this filter; not meaningful to the API. + "account_subs".into(), SubscribeRequestFilterAccounts { account: vec![pubkey.to_string()], ..Default::default() }, ); let request = SubscribeRequest { accounts, commitment: Some(self.commitment.into()), ..Default::default() }; - client::subscribe(self.laser_client_config.clone(), request).0 + // client::subscribe returns (stream, meta/unsub); we only need the stream here. + client::subscribe(self.laser_client_config.clone(), request).0 }
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs(1 hunks)test-integration/test-chainlink/tests/chain_pubsub_client.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs (3)
magicblock-chainlink/src/remote_account_provider/chain_laser_client.rs (3)
new_from_url(23-36)shutdown(81-86)subscribe(53-65)magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs (3)
new_from_url(49-56)new(58-87)shutdown(89-99)magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (7)
new(174-184)shutdown(28-28)shutdown(59-65)shutdown(261-263)subscribe(20-23)subscribe(111-124)subscribe(241-249)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run_make_ci_test
- GitHub Check: run_make_ci_test
- GitHub Check: run_make_ci_lint
🔇 Additional comments (9)
test-integration/test-chainlink/tests/chain_pubsub_client.rs (3)
9-9: LGTM! Import path updated for pubsub_common refactoring.The import path correctly reflects the move of
SubscriptionUpdateto thepubsub_commonmodule, aligning with the broader type consolidation in this PR.
39-39: LGTM! Simplified mapping improves readability.The
map_orrefactoring successfully addresses the previous review feedback and provides a cleaner, more idiomatic implementation.
98-103: LGTM! Well-documented test validates important optimization.The test correctly verifies that
update.slotmatchesclock_value.slot, and the inline comment provides valuable documentation explaining why production code can useupdate.slotdirectly instead of parsing clock data. This is excellent test-driven documentation of a performance optimization.magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs (6)
1-28: LGTM: Clean imports and well-defined type aliases.The imports are well-organized and the type aliases (
LaserResult,LaserStreamUpdate,LaserStream) improve code readability throughout the module.
33-45: LGTM: Well-structured actor with appropriate fields.The struct correctly models an actor that manages per-account gRPC subscriptions. The use of
StreamMapfor multiplexing multiple account streams is appropriate.
47-96: LGTM: Constructors follow actor pattern correctly.The two-tier constructor pattern (convenience
new_from_urland explicitnew) provides good ergonomics. Channel setup and initialization are correct.
103-131: LGTM: Run loop correctly handles channel closure and stream updates.The select! loop properly handles both message reception and subscription updates, with correct None handling for graceful shutdown (addressed from previous review).
174-206: LGTM: Subscription removal and stream creation are correct.The
remove_submethod correctly removes subscriptions and always sends a response. Thecreate_account_streamhelper properly constructs a per-account subscription request with the configured commitment level.
208-294: LGTM: Robust update handling with comprehensive error checks.The
handle_account_updatemethod includes thorough validation (pubkey mismatch check, owner parsing) and detailed error logging. The comment referencing the gist about closed accounts is helpful context. Thegrpc_commitment_from_solanahelper is straightforward and correct.Note: Failed subscriptions continue logging errors without being removed, which allows recovery from transient failures but may produce repeated error logs for persistent issues. This appears intentional for resilience.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs (3)
magicblock-chainlink/src/remote_account_provider/chain_laser_client.rs (3)
new_from_url(23-36)shutdown(81-86)subscribe(53-65)magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs (5)
new_from_url(49-56)new(58-87)shutdown(89-99)handle_msg(150-217)add_sub(219-289)magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs (7)
new(174-184)shutdown(28-28)shutdown(59-65)shutdown(261-263)subscribe(20-23)subscribe(111-124)subscribe(241-249)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run_make_ci_lint
- GitHub Check: run_make_ci_test
- GitHub Check: run_make_ci_test
magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs (2)
magicblock-chainlink/src/remote_account_provider/chain_laser_client.rs (3)
new_from_url(23-36)shutdown(81-86)subscribe(53-65)magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs (5)
new_from_url(49-56)new(58-87)shutdown(89-99)handle_msg(150-217)add_sub(219-289)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: run_make_ci_format
- GitHub Check: run_make_ci_test
- GitHub Check: run_make_ci_test
- GitHub Check: run_make_ci_lint
🔇 Additional comments (1)
magicblock-chainlink/src/remote_account_provider/chain_laser_actor.rs (1)
300-305: No changes needed. The code correctly follows LaserStream's recommended error-handling pattern.LaserStream has built-in automatic reconnection with configurable retry behavior and backoff, and the recommended client behavior is to log errors by relying on the SDK's reconnect. Your code at lines 300-305 does exactly this: it logs the error and lets the SDK handle recovery. The concern about repeated error logs is mitigated by the SDK's backoff behavior. Additionally, when a stream permanently fails (returns
None), theStreamMapwill automatically remove it on the next poll cycle.
| /// Helper to create a dedicated stream for a single account. | ||
| fn create_account_stream( | ||
| &self, | ||
| pubkey: Pubkey, | ||
| ) -> impl Stream<Item = LaserResult> { | ||
| let mut accounts = HashMap::new(); | ||
| accounts.insert( | ||
| "account_subs".into(), | ||
| SubscribeRequestFilterAccounts { | ||
| account: vec![pubkey.to_string()], | ||
| ..Default::default() | ||
| }, | ||
| ); | ||
| let request = SubscribeRequest { | ||
| accounts, | ||
| commitment: Some(self.commitment.into()), | ||
| ..Default::default() | ||
| }; | ||
| client::subscribe(self.laser_client_config.clone(), request).0 | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider the performance impact of cloning laser_client_config per subscription.
The client::subscribe call clones the entire LaserstreamConfig (including API key, endpoint URL, and channel options) for each account subscription. While this is likely required by the helius-laserstream API's ownership model, it could have performance implications with many subscriptions.
If the helius-laserstream library supports connection pooling or shared client instances, consider refactoring to reuse a single client or config reference. Otherwise, this is acceptable as-is given API constraints.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
magicblock-chainlink/src/chainlink/mod.rs(2 hunks)test-integration/test-chainlink/tests/ix_remote_account_provider.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run_make_ci_lint
- GitHub Check: run_make_ci_test
- GitHub Check: run_make_ci_test
🔇 Additional comments (3)
test-integration/test-chainlink/tests/ix_remote_account_provider.rs (2)
6-6: Import migration completed correctly.The import has been properly updated to use
ChainUpdatesClient, which is the new unified enum that supports both WebSocket and Laser gRPC clients.
24-26: Function signature correctly updated.The return type properly reflects the migration to
SubMuxClient<ChainUpdatesClient>, maintaining consistency with the new client enum.magicblock-chainlink/src/chainlink/mod.rs (1)
20-22: Imports align with the new updates backend.Bringing
chain_updates_client::ChainUpdatesClientalongside theChainPubsubClienttrait is consistent with the refactor. No issues spotted here.
| V, | ||
| C, | ||
| >, | ||
| Chainlink<ChainRpcClientImpl, SubMuxClient<ChainUpdatesClient>, V, C>, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
Stabilize the public return type with an alias to avoid downstream churn.
Exposing SubMuxClient<ChainUpdatesClient> in the public signature leaks backend details and may force downstream changes next time the backend evolves. Add a stable alias and use it in the signature.
Apply:
@@
- ) -> ChainlinkResult<
- Chainlink<ChainRpcClientImpl, SubMuxClient<ChainUpdatesClient>, V, C>,
- > {
+ ) -> ChainlinkResult<
+ Chainlink<ChainRpcClientImpl, ChainUpdatesMuxClient, V, C>,
+ > {And define the alias once in this module (near other pub uses):
@@
pub use blacklisted_accounts::*;
+/// Stable alias for the multiplexed updates client to keep backend details abstract.
+pub type ChainUpdatesMuxClient = SubMuxClient<ChainUpdatesClient>;Run to find other occurrences that could switch to the alias:
🏁 Script executed:
#!/bin/bash
rg -nP --type=rust -C1 'SubMuxClient\s*<\s*ChainUpdatesClient\s*>'Length of output: 3562
Define and use a public type alias to stabilize the chain updates client interface.
The pattern SubMuxClient<ChainUpdatesClient> appears across 14 locations in the codebase (test-integration, magicblock-api, magicblock-chainlink, magicblock-aperture, magicblock-accounts). Exposing this concrete backend type in public signatures creates tight coupling and forces downstream changes when the implementation evolves.
Suggested approach:
Define the alias in magicblock-chainlink/src/chainlink/mod.rs:
pub use blacklisted_accounts::*;
+/// Stable alias for the multiplexed updates client to keep backend details abstract.
+pub type ChainUpdatesMuxClient = SubMuxClient<ChainUpdatesClient>;Update the public return type at line 92:
- ) -> ChainlinkResult<
- Chainlink<ChainRpcClientImpl, SubMuxClient<ChainUpdatesClient>, V, C>,
- > {
+ ) -> ChainlinkResult<
+ Chainlink<ChainRpcClientImpl, ChainUpdatesMuxClient, V, C>,
+ > {Additional refactoring opportunities: Consider updating other public signatures in magicblock-chainlink/src/remote_account_provider/mod.rs (lines 172, 184, 275, 302, 960) and similar public exports across other modules to use the alias for consistency.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Chainlink<ChainRpcClientImpl, SubMuxClient<ChainUpdatesClient>, V, C>, | |
| Chainlink<ChainRpcClientImpl, ChainUpdatesMuxClient, V, C>, |
🤖 Prompt for AI Agents
magicblock-chainlink/src/chainlink/mod.rs around line 92: define a public type
alias such as pub type ChainUpdatesBackend = SubMuxClient<ChainUpdatesClient>;
then replace the exposed concrete type usage at line 92
(Chainlink<ChainRpcClientImpl, SubMuxClient<ChainUpdatesClient>, V, C>) with the
alias (Chainlink<ChainRpcClientImpl, ChainUpdatesBackend, V, C>) to decouple the
public API from the concrete implementation; also scan and update other public
signatures in magicblock-chainlink/src/remote_account_provider/mod.rs (lines
172, 184, 275, 302, 960) and other modules cited to use the same alias to
stabilize the interface.
| RemoteAccountProvider::< | ||
| ChainRpcClientImpl, | ||
| SubMuxClient<ChainPubsubClientImpl>, | ||
| SubMuxClient<ChainUpdatesClient>, |
There was a problem hiding this comment.
🧩 Analysis chain
Type instantiation is correct, but verify main implementation file is fully migrated.
The type parameter update in this test file is correct and consistent. However, based on previous review findings, the main implementation file magicblock-chainlink/src/remote_account_provider/mod.rs may still have incomplete migration with three impl blocks (around lines 159, 940, and 952) still referencing ChainPubsubClientImpl instead of ChainUpdatesClient.
Please verify that all impl blocks in the main file have been updated to prevent potential type mismatches or compilation errors.
Run the following script to verify that no remaining ChainPubsubClientImpl references exist in impl blocks:
🏁 Script executed:
#!/bin/bash
# Check for any remaining ChainPubsubClientImpl references in the main implementation
echo "=== Checking for ChainPubsubClientImpl in impl blocks ==="
rg -nP 'impl.*ChainPubsubClientImpl' magicblock-chainlink/src/remote_account_provider/mod.rs
echo ""
echo "=== All RemoteAccountProvider impl signatures ==="
rg -n -A2 'impl.*RemoteAccountProvider' magicblock-chainlink/src/remote_account_provider/mod.rs | head -50
echo ""
echo "=== Verify ChainUpdatesClient is used consistently ==="
rg -n 'SubMuxClient<ChainUpdatesClient>' magicblock-chainlink/src/remote_account_provider/mod.rsLength of output: 870
I need to inspect the impl block at line 940 more closely to understand the full scope of what needs to be updated.
🏁 Script executed:
#!/bin/bash
# Get context around the problematic impl block at line 940
echo "=== Context around line 940 impl block ==="
sed -n '935,975p' magicblock-chainlink/src/remote_account_provider/mod.rs
echo ""
echo "=== Check if there are other impl blocks that still need updating ==="
rg -n 'impl.*RemoteAccountProvider.*ChainPubsubClientImpl' magicblock-chainlink/src/remote_account_provider/mod.rsLength of output: 308
I need to find the correct path to the implementation file first.
🏁 Script executed:
#!/bin/bash
# Find the remote_account_provider file
echo "=== Locating remote_account_provider implementation ==="
find . -name "mod.rs" -path "*remote_account_provider*" 2>/dev/null
echo ""
echo "=== Alternative: search for ChainPubsubClientImpl references ==="
rg -l 'ChainPubsubClientImpl' --type rust 2>/dev/null | head -10Length of output: 441
🏁 Script executed:
#!/bin/bash
# Get context around the problematic impl block at line 940
echo "=== Context around line 940 impl block ==="
sed -n '935,975p' ./magicblock-chainlink/src/remote_account_provider/mod.rs
echo ""
echo "=== Count of ChainPubsubClientImpl references in main file ==="
rg -c 'ChainPubsubClientImpl' ./magicblock-chainlink/src/remote_account_provider/mod.rsLength of output: 1025
🏁 Script executed:
#!/bin/bash
# Check if the old impl blocks are actually used anywhere
echo "=== Search for RemoteAccountProvider instantiations with ChainPubsubClientImpl ==="
rg -n 'RemoteAccountProvider.*ChainPubsubClientImpl' --type rust -g '!mod.rs' | head -20
echo ""
echo "=== Check test file for what type it actually instantiates ==="
sed -n '30,40p' ./test-integration/test-chainlink/tests/ix_remote_account_provider.rs
echo ""
echo "=== Verify no compilation would fail due to ambiguous impl blocks ==="
rg -n 'RemoteAccountProvider::<' ./test-integration/test-chainlink/tests/ix_remote_account_provider.rsLength of output: 606
Remove unused old impl blocks in main implementation file.
The test file changes are correct—they properly use SubMuxClient<ChainUpdatesClient>. However, the main implementation file still contains two dead code impl blocks for the old type that should be removed:
- Line 940-942:
impl RemoteAccountProvider<ChainRpcClientImpl, ChainPubsubClientImpl>(unused) - Line 951-967:
impl RemoteAccountProvider<ChainRpcClientImpl, SubMuxClient<ChainPubsubClientImpl>>(unused)
These blocks are never instantiated anywhere in the codebase. Remove them to complete the migration and eliminate confusion. Keep only the impl block at line 960+ that uses SubMuxClient<ChainUpdatesClient>.
🤖 Prompt for AI Agents
In test-integration/test-chainlink/tests/ix_remote_account_provider.rs around
lines 32-34, remove the two dead/unused impl blocks in the main implementation
file: delete the impl block at lines 940-942 (impl
RemoteAccountProvider<ChainRpcClientImpl, ChainPubsubClientImpl>) and the impl
block at lines 951-967 (impl RemoteAccountProvider<ChainRpcClientImpl,
SubMuxClient<ChainPubsubClientImpl>>); ensure you keep the existing impl block
at line ~960+ that uses SubMuxClient<ChainUpdatesClient>, run cargo check/tests
to confirm no references remain, and update any imports/doc comments if they now
reference the removed types.
| } | ||
| } | ||
|
|
||
| pub fn is_helius_laser_url(url: &str) -> bool { |
There was a problem hiding this comment.
I think having a separate config instead of reusing the ws-url is clearer and less confusing. It also leaves room to connect to both in the future for redundancy.
Summary
This PR integrates Helius Laser gRPC as a new endpoint option for real-time account subscriptions, providing a more reliable and efficient alternative to WebSocket-based pubsub connections. It also adds a fully automated manual test suite to validate the integration end-to-end.
Details
Helius Laser gRPC Integration
The main change introduces support for Helius Laser gRPC endpoints as an alternative to WebSocket pubsub connections. This is particularly beneficial for high-reliability deployments where stable, long-lived connections are critical.
Sample Configuration
To use Helius Laser gRPC, configure your validator with:
For mainnet, use the mainnet endpoints instead of devnet ones.
Manual Test Suite
A comprehensive manual test suite (
test-manual/) validates the laser gRPC integration:Technical Improvements
SubscriptionUpdatestructDeps
Summary by CodeRabbit
New Features
Improvements
Tests